3주차 리뷰 부탁드립니다. #11
3주차 리뷰 부탁드립니다. #11
Conversation
week3 인증관련 구현 사항
Week3 프로젝트 관련 구현사항
김도헌 3주차 weekly 반영
김동혁 3주차 weekly 반영
조서영 3주차 기능 구현
권순호 3주차 weekly 반영
3주차 weekly -> develop
3주차 develop -> master
| - style : 코드 스타일 변경 (포매팅 수정, 세미콜론 추가 등) | ||
| - refactor : 코드 리팩토링 | ||
| - test : 테스트 코드 추가, 수정 | ||
| - chore : 빌드 프로세스, 도구 설정 변경 등 기타 작업 |
| - style : 코드 스타일 변경 (포매팅 수정, 세미콜론 추가 등) | ||
| - refactor : 코드 리팩토링 | ||
| - test : 테스트 코드 추가, 수정 | ||
| - chore : 빌드 프로세스, 도구 설정 변경 등 기타 작업 | ||
|
|
||
| --- | ||
|
|
||
| # 구현 기능 목록 |
| // OAuth2에서 가져온 유저 정보 | ||
| public static class OAuthAttributes { | ||
|
|
||
| private Map<String, Object> attributes; |
There was a problem hiding this comment.
Map<String, Object>로 두는 것은 좋지 않습니다.
특히 Object를 사용하면 타입 체크가 컴파일 시점에 이루어지지 않아 버그를 발생시킬 수 있습니다.
가능하다면 명시적인 필드를 가진 클래스를 사용하는 것이 어떨까요?
There was a problem hiding this comment.
oauth로 부터 .getAttributes()를 받아오기 때문에 데이터 타입을 유지해야 합니다.
인증 플랫폼마다 반환되는 결과가 달라서 각자 값만 받아오기도 어렵습니다.
| @@ -0,0 +1,117 @@ | |||
| package com.example.team1_be.DTO; | |||
There was a problem hiding this comment.
자바 패키지 이름 규칙에 따르면 언더스코어(_) 대신 카멜 케이스를 사용하는 것이 좋아요
| // OAuth2User 반환용 | ||
| public record PrincipalDetails( | ||
| UserEntity user, | ||
| Map<String, Object> attributes, |
| } | ||
|
|
||
| @Transactional | ||
| protected UserEntity saveOrUpdate(OAuthAttributes attributes) { |
There was a problem hiding this comment.
동일 클래스 내에서 사용하는 경우 private으로 선언하는 것이 좋습니다
There was a problem hiding this comment.
https://docs.spring.io/spring-framework/reference/data-access/transaction.html
@transactional이 private면 프록시 객체 메소드를 실행할 수 없어서 protected로 했습니다.
|
|
||
| public Project updateProject(long get, ProjectDTO.update update) { | ||
| Project project = projectRepository.findById(get) | ||
| .orElseThrow(() -> new BaseHandler(HttpStatus.NOT_FOUND, "존재하지 않음")); |
There was a problem hiding this comment.
에러 메시지는 "프로젝트가 존재하지 않음"과 같이 구체적으로 설명하는 것이 좋습니다.
| return project.getGuests(); | ||
| } | ||
|
|
||
| public Project createProject(ProjectDTO.create create) { |
There was a problem hiding this comment.
@Transactional 어노테이션을 활용하면 좋겠습니다!
(createProject 외에도 많은 변경, 생성이 일어나는 메서드)
There was a problem hiding this comment.
@Transactional은 보통 Service Layer에서만 적용을 하면 되나요? 제가 알기로는 JpaRepository는 @Transactional를 포함하고 있고, Controller의 경우 데이터 변환이 일어나지 않기 때문에 위와 같이 생각했습니다. 멘토님은 어떠신가요?
There was a problem hiding this comment.
네 @Transactional은 보통 Service 계층에 적용합니다. (repository, controller 모두 말씀해주신 부분이 맞습니다.)
| project.setGuests(update.getGuests()); | ||
| project.setOptions(update.getOptions()); | ||
| project.setStartDate(update.getStartDate()); | ||
| project.setEndDate(update.getEndDate()); |
There was a problem hiding this comment.
여러 번의 set 호출 대신 더 나은 방법을 고민해보시면 좋을 것 같아요.
There was a problem hiding this comment.
update의 경우 기존의 Entity를 id로 조회하여 가져오고. 해당 Entity의 몇몇 field의 정보를 변경하는 것이라서 이렇게 구현을 했습니다. 멘토님의 말을 듣고 생각해보니 ProjectEntity에 void updateProject()라는 메소드를 만들어 생성자 초기화하듯이 업데이트를 구현할 수도 있을거 같네요 멘토님은 어떠하신가요?
There was a problem hiding this comment.
여러 번의 setter 호출 대신, 엔티티에 update를 위한 메서드를 만들어서 사용하는 것이 좋습니다.
-
여러 setter를 사용하면 객체가 일관성 없는 상태로 사용될 위험이 있습니다.
예를 들어, 지금 updateProject() 내에서 set, set, set으로 정보를 변경하는 로직을 짰는데
이후 Project 객체에 새로운 필드가 추가되면서 해당 필드에 대한 set을 누락할 가능성이 있습니다.
새로운 필드가 여러 개라면 더욱 누락할 위험이 높아요. -
객체 지향 원칙에 위배됩니다.
외부에서 직접 객체 내부 상태를 (setter로) 변경할 수 있게 되면서 캡슐화가 약해집니다.
추가로 이 메서드에 @Transactional을 추가하면 Jpa 변경 감지 기능 덕분에 엔티티의 변경사항이 자동으로 데이터베이스에 반영되어 아래 save 메서드를 호출할 필요가 없습니다.
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| public class UserMapper { |
|
week3 궁금한점 (리뷰 부탁드립니다.)